-
Notifications
You must be signed in to change notification settings - Fork 112
Add Sourcebot Helm Chart #370
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
Signed-off-by: Andre Nogueira <[email protected]>
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
This is awesome thanks Andre! Let us know if there's anything we can help with to get this PR out of drafts |
The chart it self should be good to go. In order to use it, the typical way is to provide a way to fetch the packaged chart. This means hosting it on a registry. Alternatively, Github pages can be used for this - here is the documentation reference: https://helm.sh/docs/topics/chart_repository/ Added it as a Draft to discuss which approach would work best here. |
@aanogueira I haven't looked at the code yet (I'll be happy to give a review if you're interested ; we've built our own Helm chart internally and I was also considering contributing it to the community) but I was wondering if you were able to run Sourcebot in Kubernetes just as is with the embedded postgres server (starting from V3)? Asking because we faced errors related to the volume mount (#263) and had to use a separate Postgres in the end. |
kubernetes/chart/README.md
Outdated
| command | list | `[]` | Override the default command of the container. | | ||
| config | object | `{"$schema":"https://raw.githubusercontent.com/sourcebot-dev/sourcebot/main/schemas/v3/index.json","connections":{},"settings":{}}` | Configure Sourcebot-specific application settings. | | ||
| containerSecurityContext | object | `{}` | Set the container-level security context. | | ||
| database | object | `{}` | Configure the database secret. | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reading this, it's unclear what is expected in the database
key.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's in the values.yaml
, but added to the generated docs as well for clarity.
kubernetes/chart/README.md
Outdated
| image | object | `{"pullPolicy":"Always","repository":"ghcr.io/sourcebot-dev/sourcebot","tag":"latest"}` | Configure the container image. | | ||
| image.pullPolicy | string | `"Always"` | Image pull policy. | | ||
| image.repository | string | `"ghcr.io/sourcebot-dev/sourcebot"` | Container image repository. | | ||
| image.tag | string | `"latest"` | Container image tag. | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we want to default to "latest"? It's usually a bad practice to use "latest" as you can't be sure of which version is deployed and it's not reproducible. I would stick a fixed version here by default, and let user change it optionally.
This also means that the release workflow should include bumping the default version of the image in the container and release a new version of the Helm chart.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was me testing it out, and forgot to remove it from the default values.
Added the specific version to the appVersion
section of the Chart.yaml
metadata.
kubernetes/chart/README.md
Outdated
| ingress.hosts | list | `[]` | List of hostnames and paths for ingress rules. | | ||
| ingress.tls | list | `[]` | TLS settings for ingress. | | ||
| initContainers | list | `[]` | Configure init containers to run before the main container. | | ||
| license | object | `{}` | Configure the enterprise license key secret. | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here, unclear what kind of value is expected.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's in the values.yaml
, but added to the generated docs as well for clarity.
kubernetes/chart/README.md
Outdated
| readinessProbe.httpGet.port | string | `"http"` | Port to check. | | ||
| readinessProbe.initialDelaySeconds | int | `10` | Initial delay before the first probe. | | ||
| readinessProbe.periodSeconds | int | `10` | Frequency of the probe. | | ||
| redis | object | `{}` | Configure the Redis secret. | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same comment here :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's in the values.yaml
, but added to the generated docs as well for clarity.
kubernetes/chart/README.md
Outdated
| startupProbe.periodSeconds | int | `30` | Initial delay before the first probe. | | ||
| tolerations | list | `[]` | Set tolerations for pod scheduling. See: https://kubernetes.io/docs/concepts/scheduling-eviction/taint-and-toleration/ | | ||
| volumeMounts | list | `[]` | Define volume mounts for the container. | | ||
| volumes | list | `[]` | Define additional volumes. | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should probably give some guidance as to what to configure for volumes (especially path expected by Sourcebot). Unless I misread, if nothing is specified, there's no persistence in the current version of the chart.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The volumes
/volumeMounts
follow the default Kubernetes specs.
As for persistence, I added support on the new commit to the storage
option to support the usage of the internal DB, mounting to the pod under /data
path.
No, there was no persistence. For live deployments, I was not assuming the usage of the internal DB, but indeed there might be some scenario where it could be useful. For those cases, we could deploy a StatefulSet instead of Deployment. Added a new block (storage) to add persistence storage, changing the deployable from a Deployment to a StatefulSet. |
Signed-off-by: Andre Nogueira <[email protected]>
This PR adds an Helm Chart enabling the deployment of Sourcebot to Kubernetes.